Distinguish muted users in UI#1429
Conversation
c34b94b to
d80442b
Compare
86d0e66 to
db3216f
Compare
db3216f to
17f2f6b
Compare
8ab889f to
cddcfcc
Compare
|
@chrisbobbe This is now ready for a general review, as we discussed on the call this week. PTAL. I am now starting to write tests. |
There was a problem hiding this comment.
This icon file is displayed differently when used as ZulipIcon. In the file, the head section is an outlined circle, but in the app, it is a filled circle.
There was a problem hiding this comment.
Yeah, fill-rule="evenodd" doesn't work when converted to a font. Here's instructions for fixing it:
https://zulip.readthedocs.io/en/latest/subsystems/icons.html#correcting-icons-with-an-evenodd-fill-rule
cddcfcc to
3df45ad
Compare
|
Thanks, glad to see this progress on this helpful feature! Comments on the first four commits: 451ec5a api: Add InitialSnapshot.mutedUsers Let's put the API-binding changes in their own case MutedUsersEvent():
// TODO handlebut that's easy to do. 🙂 Then please send those revised commits as a new, non-draft PR and label it for Greg's review:
Thanks! I'll start reading the later commits now. |
chrisbobbe
left a comment
There was a problem hiding this comment.
And here's a review of the next commit 🙂:
70722283a msglist: Distinguish messages sent by muted users
There was a problem hiding this comment.
Interesting; the ValueChanged typedef was new to me!
I think a different approach would be worthwhile, though, toward more familiar/transparent state management and to minimize added code in _MessageListPageState.build.
-
On
MessageListPageState, add methods like the following:/// The "revealed" state of a message from a muted sender. /// /// See also: [revealMutedMessage], [unrevealMutedMessage]. bool isMutedMessageRevealed(int messageId); /// For a message from a muted sender, reveal the sender and content, /// replacing the "Muted sender" placeholder. void revealMutedMessage(int messageId); /// For a message from a muted sender, hide the sender and content again /// with the "Muted sender" placeholder. void unrevealMutedMessage(int messageId);
-
Implement these in
_MessageListPageState, backed by aSetof message IDs. The implementations should be simple and don't need to involve the store or any event handling; just simple add/remove/read. -
Read the dartdoc on
MessageListPage.ancestorOfand the implementation comment under it. Since we'll need to call this from a build method, make a prep commit that creates anInheritedWidget, so thatancestorOfcan become something like:/// The [MessageListPageState] above this context in the tree. /// /// Uses the efficient [BuildContext.dependOnInheritedWidgetOfExactType], /// so this may be called in a build method. static MessageListPageState ancestorOf(BuildContext context) { final state = context .dependOnInheritedWidgetOfExactType<_MessageListPageInheritedWidget>() .state; assert(state != null, 'No _MessageListPageInheritedWidget ancestor'); return state!; }
Ah—in fact I think it might need to be an
InheritedNotifier, with thereveal…/unreveal…methods callingnotifyListeners, so that widgets update whenisMutedMessageRevealedhas a new value? -
HideMutedMessageButton(perhaps renamedUnrevealMutedMessageButtonfor consistency) can just do this in itsonPressed:MessageListPage.ancestorOf(pageContext).unrevealMutedMessage(message.id);
-
The "Reveal" button in the message list can do this:
MessageListPage.ancestorOf(context).revealMutedMessage(message.id);
-
And the UI state in
_MessageWithPossibleSenderState.buildcan useMessageListPage.ancestorOf(context).isMutedMessageRevealed.
There was a problem hiding this comment.
Implement these in _MessageListPageState, backed by a Set of message IDs. The implementations should be simple and don't need to involve the store or any event handling; just simple add/remove/read.
I started to refactor the code based on the suggested changes, but it's getting complex; maybe I am not doing it the right way. Also, I think we still need store to react to live changes via MutedUsersEvent.
There was a problem hiding this comment.
I see; yeah, I'm running into trouble with the InheritedNotifier approach too. I'll see if I can make something else work.
There was a problem hiding this comment.
This feels verbose; how about just "Reveal message"? The sender row already says "Muted sender".
There was a problem hiding this comment.
How about "Muted user"? The fact that it's the sender is already communicated by the position of this text.
There was a problem hiding this comment.
I think "Muted user" is better for the reason you mentioned, and I think Web should also consider using this phrase in #34794.
There was a problem hiding this comment.
Ooh good thought; I've left a comment there. Thanks for noticing!
There was a problem hiding this comment.
msglist: Distinguish messages sent by muted users
Commit-message nit: no Fixes: line until a later commit, when all of the issue's work is done :)
There was a problem hiding this comment.
If all callers pass size, let's make it required
There was a problem hiding this comment.
What's the meaning of the size param, and why isn't it used directly here? When I read this I wonder why callers don't just pass the size they want (e.g. from a Figma frame) and expect it to be applied faithfully :)
There was a problem hiding this comment.
Ah I think I see: size is really about the dimensions of the square gray box, and the icon is meant to be some fraction of that, right?
Oh, but a nuance: the actual size of the box is controlled by AvatarShape, which callers are supposed to use as a wrapper. So I think the size param calls for a dartdoc explaining why it's needed; something like:
/// The size of the placeholder box.
///
/// This should match the `size` passed to the wrapping [AvatarShape].
/// The placeholder's "person" icon will be scaled proportionally to this.Then here, on the arithmetic, an implementation comment like:
// Where the avatar placeholder appears in the Figma,
// this is how the icon is sized proportionally to its box.There was a problem hiding this comment.
Ah, these aren't Figma design variables in the usual way—
—they're part of a "color palette" (see #831).
Since the Figma doesn't seem to offer a variable, let's first see how these look in dark mode (please post screenshots); if they look reasonable (even if not perfect), I'd just hard-code the colors inline, with a TODO(design): dark-mode variant?. If we need a dark-mode variant, please @-mention Vlad in #mobile.
There was a problem hiding this comment.
There was a problem hiding this comment.
Looking at #831, which will pull those colors automatically, should we remove them from theme.dart and use them inline for now?
There was a problem hiding this comment.
Does it look OK if we use listMenuItemBg for the rounded-rectangle background and listMenuItemIcon for the person icon? With a TODO in the widget code like:
// TODO(design) this was ad-hoc, we need a new variableHere they are in a screenshot from the Figma:
Here are the hex values (from hovering over where it says grey/200 etc.):
| Name | Light | Dark |
|---|---|---|
listMenuItemBg |
#cbcdd6 |
#2d303c |
listMenuItemIcon |
#9194a3 |
#767988 |
This would mean adding listMenuItemBg and listMenuItemIcon to DesignVariables. It does seem like we can't use the same color for both light and dark theme, and that means we have to use DesignVariables, which enables the smooth animation between light and dark theme.
There was a problem hiding this comment.
That does seem like a better fit! Thanks for finding those. Since those aren't named variables from the Figma—they're in the section with the comment
// Not named variables in Figma; taken from older Figma drafts, or elsewhere.—we're free to name them however we want. Could say avatarPlaceholderIcon and avatarPlaceholderBg; I think that would fit these new uses as well as the existing use on the recent-DMs page.
There was a problem hiding this comment.
Pushed with these new changes.
There was a problem hiding this comment.
With 20+ lines of code, this would be good to pull out into a separate widget or a helper method
3df45ad to
c1a1982
Compare
|
"Reveal message for muted sender" is an odd phrase. I think @terpimost 's design just had "Reveal":
Was this changed because we decided to put the button on a separate line from the sender? Even so, maybe "Reveal message" is best -- I think it'll be clear enough. |
There was a problem hiding this comment.
Here's another thought, from looking again at #296: can we centralize the code that chooses "Muted user" to replace a user's name?
Because the list of places that should be affected is so long, we should find ways to encapsulate many of those to go through a much smaller number of distinct places in the source code; otherwise it'll be very easy to miss some, and to forget this feature when adding new pieces of UI that happen to show a user's name or avatar.
What do you think of this commit sequence:
- In one commit (or more if it gets complicated), make sure
store.userDisplayNameandstore.senderDisplayNameare used in all places that show a user's name - Adjust
store.userDisplayNameandstore.senderDisplayNameso they give "Muted user" (translated) when the user is muted. If any callers still want the real name when the user is muted, you can add a param likereplaceIfMuted(default true). - Other commits that change how we show muted users, like the reveal/unreveal logic for messages in the message list
- Commits that exclude muted users from the UI
- There's another of these: the web app excludes muted users in the typing-status row in the message list; let's do that too.
| }, | ||
| "mutedUser": "Muted user", | ||
| "@mutedUser": { | ||
| "description": "Name for a muted user to display all over the app." |
There was a problem hiding this comment.
nit: how about "Text to display in place of a muted user's name"
(I think "all over the app" isn't very actionable for translators; the important thing is that it replaces strings like "Chris Bobbe", "Greg Price" etc.; it doesn't matter as much that those strings appear all over the app)
Also renamed "user" to "two_person" to make it consistent with other icons of the same purpose. New icons taken from: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=5968-237884&t=dku3J5Fv2dmWo7ht-0 https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=5968-264632&t=dku3J5Fv2dmWo7ht-0 https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=5968-291260&t=B5jAmeUMMG4dlsU7-0
9ca9390 to
55e46ff
Compare
|
Thanks @chrisbobbe for the previous draft review! This is now ready for a complete review. I have addressed Greg's review on #1530 and included those commits too. I will now start addressing your last review. Note: The PR is giving the following CI/CD error, and I am not sure what it is about: |
@alya We had a discussion in #mobile-design > muted users @ 💬 and decided to use "Reveal message for muted sender", but I think it's longer and the newer version (Reveal message) is better. |
|
I suggested "Reveal message" above, so yes, that's fine. |
7eabd1e to
00cce74
Compare
|
Thanks @chrisbobbe for the previous reviews. I have addressed all comments but 1429 comment and 1429 comment from the previous review. For the first one, I left a comment below it. I will continue addressing these comments once I am back from the holiday. |
The places are: - App bar title in DM narrow - DM recipient header
Conversations are excluded where all other recipients are muted.
This is solely for a better order.
Direct messages of those conversations are excluded where all of the other recipients are muted. Currently the applicable narrows are: - CombinedFeedNarrow - MentionsNarrow - StarredMessagesNarrow In the future, this will apply to the ReactionsNarrow from the Web app too, once we have it.
00cce74 to
50c6764
Compare
Following web: zulip#1429 (review) This completes the planned work for muting muted users (hooray!). Fixes: zulip#296
|
I ended up making separate PRs to incorporate my feedback directly, with the launch coming so soon and since you've been on vacation. :) So I'll close this as superseded by those PRs: Ah and I guess the prep PR #1530 is still open, as discussed at #GSoC > Sayed Mahmood - Check-ins @ 💬—it would be great if you could do the next revision of that. |





Note: This PR is rebased on top of #1530.
This PR aims to respect the user's settings for muting other users by distinguishing its related UI appearance. From the list of places in UI mentioned in https://zulip.com/help/mute-a-user, the following are included as they're compatible with the current scope of mobile:
composing a direct message ormentioning a user.Fixes: #296